Allow complex values in variables parameter of terraform module #4281

wmudge · 2022-02-23T20:45:07Z

Signed-off-by: Webster Mudge [email protected]

SUMMARY

Allow complex, i.e. lists and dictionaries, values in the variables parameter of the terraform module. Without this minor patch, complex values will fail due to formatting issues, i.e. Single quotes are not valid. Use double quotes (\") to enclose strings.

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

community.general.terraform

Signed-off-by: Webster Mudge <[email protected]>

ansibullbot · 2022-02-23T20:52:31Z

cc @m-yosefpor @rainerleber
click here for bot help

felixfontein

Thanks for your contribution! Can you please add a changelog fragment? Thanks.

felixfontein · 2022-02-23T20:59:33Z

plugins/modules/cloud/misc/terraform.py

@@ -443,7 +443,7 @@ def main():
    for k, v in variables.items():
        variables_args.extend([
            '-var',
-            '{0}={1}'.format(k, v)
+            '{0}={1}'.format(k, json.dumps(v))


How does terraform interpret arguments of -var? I'm wondering whether this is a breaking change or not.

Could you please comment on this one?

I don't believe so, as I haven't hit any issues with this patch in my use cases. That said, I just reread the TF docs on the topic (see Input Variables on the Command Line and Assigning Values to Root Module Variables), and the references all seem to point to TF "native" expressions. However, JSON Configuration Syntax states that "Everything that can be expressed in native syntax can also be expressed in JSON syntax, but some constructs are more complex to represent in JSON due to limitations of the JSON grammar."

I'll run some more tests to see if I can break this, but I would think that if you have really complex variables, you are best served by creating your own variables file and passing that as a reference. The intention of this patch is to allow "simple" complex variables, like list(string), to be declared in the variables parameter.

Thanks for your explanations and links! Sounds convincing to me. (I'm not using terraform so I can't test myself.)

This has turned out to be a breaking change. See #4367.

Signed-off-by: Webster Mudge <[email protected]>

changelogs/fragments/4281-terraform-complex-variables.yml

Co-authored-by: Felix Fontein <[email protected]>

felixfontein · 2022-03-02T21:23:02Z

The change looks reasonable to me. If nobody complains, I'll merge in ~a week.

patchback · 2022-03-10T20:38:32Z

Backport to stable-3: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-3/4cc7f413953606cda79596c7682eac0319936199/pr-4281

Backported as #4340

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

felixfontein · 2022-03-10T20:38:40Z

@wmudge thanks a lot for your contribution!

* Allow complex values in variables parameter Signed-off-by: Webster Mudge <[email protected]> * Add changelog fragment Signed-off-by: Webster Mudge <[email protected]> * Update changelogs fragments formatting Co-authored-by: Felix Fontein <[email protected]> Co-authored-by: Felix Fontein <[email protected]> (cherry picked from commit 4cc7f41)

patchback · 2022-03-10T20:38:46Z

Backport to stable-4: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-4/4cc7f413953606cda79596c7682eac0319936199/pr-4281

Backported as #4341

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

* Allow complex values in variables parameter Signed-off-by: Webster Mudge <[email protected]> * Add changelog fragment Signed-off-by: Webster Mudge <[email protected]> * Update changelogs fragments formatting Co-authored-by: Felix Fontein <[email protected]> Co-authored-by: Felix Fontein <[email protected]> (cherry picked from commit 4cc7f41)

… (#4340) * Allow complex values in variables parameter Signed-off-by: Webster Mudge <[email protected]> * Add changelog fragment Signed-off-by: Webster Mudge <[email protected]> * Update changelogs fragments formatting Co-authored-by: Felix Fontein <[email protected]> Co-authored-by: Felix Fontein <[email protected]> (cherry picked from commit 4cc7f41) Co-authored-by: Webster Mudge <[email protected]>

… (#4341) * Allow complex values in variables parameter Signed-off-by: Webster Mudge <[email protected]> * Add changelog fragment Signed-off-by: Webster Mudge <[email protected]> * Update changelogs fragments formatting Co-authored-by: Felix Fontein <[email protected]> Co-authored-by: Felix Fontein <[email protected]> (cherry picked from commit 4cc7f41) Co-authored-by: Webster Mudge <[email protected]>

earchibald-lv · 2022-03-15T18:08:22Z

This has completely broken my existing playbooks. See #4367.

2 hours of "what could possibly have changed, what do you mean "us-east-1" is an invalid region?" later...

earchibald-lv · 2022-03-15T18:20:37Z

You could have simply told people to pass complex values through the to_json filter on their own.

This "fix" forces everything through json.dumps() and completely incorrectly escapes things that should NOT BE escaped.

earchibald-lv · 2022-03-15T18:49:45Z

Specifically, this breaks PLAIN STRINGS.

"string" becomes json-escaped: "\"string\"", which fails at the terraform layer.

Complex data that has already gone through a to_json filter is DOUBLE JSON-escaped, which fails at the terraform layer.

mkjpryor · 2022-03-15T20:02:40Z

@earchibald-lv is correct - this change is not just not backwards compatible, but is also actually totally broken in the way that it now handles string values.

I have some code that uses the terraform module that has variables of type string and variables of type list(string). For the list(string) variables, I am using similar statements to this, which worked as of 4.5.0:

cluster_ssh_public_keys: "{{ [cluster_deploy_ssh_public_key, cluster_user_ssh_public_key] | to_json }}"

However at 4.6.0 this produces a validation error. I can fix this for 4.6.0 by switching to:

cluster_ssh_public_keys:
  - "{{ cluster_deploy_ssh_public_key }}"
  - "{{ cluster_user_ssh_public_key }}"

which is fine - in fact I probably prefer it. But I then get bitten by the following error:

Error: Error trying to get network information from the Network API: Could not find any matching network for name "portal-internal"

Now this network definitely exists, and the code was working up until yesterday so I was mystified as to what was going on, because the error message seems sensible. It took me ages to figure out that the reason it was failing is because the quotes are being treated as part of the network name.

For now, I will be sticking at 4.5.0, unfortunately.

earchibald-lv · 2022-03-15T20:44:41Z

I believe the backwards-compatible fix may be just "don't encode strings as JSON, other types are fine" but there should probably be a more comprehensive look at this before a new release.

I'd very much like a 4.6.1 with this change removed, and to see this circled back to in a later release, with time to be better-tested. I agree with @mkjpryor that being able to get rid of all the filter pipes to_json would be lovely!

mkjpryor · 2022-03-15T20:46:51Z

@earchibald-lv I think it is easier than that - the fix can be "only encode dict and list as JSON" right?

mkjpryor · 2022-03-15T20:47:37Z

There may also be others like bool and None I guess.

mkjpryor · 2022-03-15T20:48:35Z

I actually don't mind the | to_json pipes that much, as it means I am in control. I could manually build those strings if I had to in order to do something complex.

earchibald-lv · 2022-03-15T20:50:11Z

The "oh, jeez, what about that case" is the classic path to an unexpected break like this :)

I agree re: the filters and control, which skipping strings would allow*. The most important part is that it is compatible and predicable and tested.

Then begins the best practice war and that is an entirely different saga

mkjpryor · 2022-03-15T20:52:41Z

@earchibald-lv Totally agree.

IMHO, this PR should be reverted and a 4.6.1 release cut. In order to re-propose the change, we need a test suite that covers as many different types of variable as possible to ensure this doesn’t happen again. But I’m actually happy with how it is at 4.5.0 TBH.

…le (ansible-collections#4281)" This reverts commit 4cc7f41.

felixfontein · 2022-03-16T06:05:36Z

I agree this should better be reverted for now.

PR to revert this change from main and stable-3 branches: #4368. Will manually backport to stable-4 and add changelog fragment (since there has been a release from stable-4) once that's merged.

felixfontein · 2022-03-16T06:11:12Z

we need a test suite that covers as many different types of variable as possible to ensure this doesn’t happen again.

Some useful tests would be a good idea indeed. Right now there is one single unit test in tests/unit/plugins/modules/cloud/misc/test_terraform.py which checks that the module fails if called without any arguments. This isn't much more helpful than having no tests at all.

It would also be helpful if more folks could look at proposed changes and comment on them.

…le (#4281)" (#4368) This reverts commit 4cc7f41.

…le (#4281)" (#4368) This reverts commit 4cc7f41. (cherry picked from commit 9618fb9)

…le (ansible-collections#4281)" (ansible-collections#4368) This reverts commit 4cc7f41. (cherry picked from commit 9618fb9)

…le (#4281)" (#4368) (#4369) This reverts commit 4cc7f41. (cherry picked from commit 9618fb9) Co-authored-by: Felix Fontein <[email protected]>

…raform module (#4281)" (#4370) * Revert "Allow complex values in variables parameter of terraform module (#4281)" (#4368) This reverts commit 4cc7f41. (cherry picked from commit 9618fb9) * Add changelog fragment.

Allow complex values in variables parameter

27845e7

Signed-off-by: Webster Mudge <[email protected]>

wmudge changed the title ~~Allow complex values in variables parameter~~ Allow complex values in variables parameter of terraform module Feb 23, 2022

ansibullbot added bug This issue/PR relates to a bug cloud module module new_contributor Help guide this first time contributor plugins plugin (any type) small_patch Hopefully easy to review labels Feb 23, 2022

felixfontein reviewed Feb 23, 2022

View reviewed changes

felixfontein added backport-4 check-before-release PR will be looked at again shortly before release and merged if possible. labels Feb 23, 2022

Add changelog fragment

0b0f94c

Signed-off-by: Webster Mudge <[email protected]>

ansibullbot removed the small_patch Hopefully easy to review label Feb 23, 2022

felixfontein reviewed Feb 23, 2022

View reviewed changes

changelogs/fragments/4281-terraform-complex-variables.yml Outdated Show resolved Hide resolved

Update changelogs fragments formatting

57360fb

Co-authored-by: Felix Fontein <[email protected]>

ansibullbot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Feb 24, 2022

felixfontein added the backport-3 label Mar 2, 2022

felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label Mar 10, 2022

felixfontein merged commit 4cc7f41 into ansible-collections:main Mar 10, 2022

patchback bot mentioned this pull request Mar 10, 2022

[PR #4281/4cc7f413 backport][stable-3] Allow complex values in variables parameter of terraform module #4340

Merged

patchback bot mentioned this pull request Mar 10, 2022

[PR #4281/4cc7f413 backport][stable-4] Allow complex values in variables parameter of terraform module #4341

Merged

earchibald-lv mentioned this pull request Mar 15, 2022

4.6.0 has broken terraform module #4367

Closed

1 task

felixfontein added a commit to felixfontein/community.general that referenced this pull request Mar 16, 2022

Revert "Allow complex values in variables parameter of terraform modu…

2200e6d

…le (ansible-collections#4281)" This reverts commit 4cc7f41.

felixfontein mentioned this pull request Mar 16, 2022

Revert "Allow complex values in variables parameter of terraform module (#4281)" #4368

Merged

felixfontein added a commit that referenced this pull request Mar 16, 2022

Revert "Allow complex values in variables parameter of terraform modu…

9618fb9

…le (#4281)" (#4368) This reverts commit 4cc7f41.

patchback bot pushed a commit that referenced this pull request Mar 16, 2022

Revert "Allow complex values in variables parameter of terraform modu…

037a22d

…le (#4281)" (#4368) This reverts commit 4cc7f41. (cherry picked from commit 9618fb9)

patchback bot mentioned this pull request Mar 16, 2022

[PR #4368/9618fb97 backport][stable-3] Revert "Allow complex values in variables parameter of terraform module (#4281)" #4369

Merged

wmudge deleted the bugfix/terraform-complex-variables branch March 16, 2022 19:55

kosalaat mentioned this pull request Jun 14, 2022

Allow terraform module to specify complex variable structures #4797

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow complex values in variables parameter of terraform module #4281

Allow complex values in variables parameter of terraform module #4281

wmudge commented Feb 23, 2022

ansibullbot commented Feb 23, 2022

felixfontein left a comment

felixfontein Feb 23, 2022

felixfontein Mar 1, 2022

wmudge Mar 1, 2022

felixfontein Mar 1, 2022

earchibald-lv Mar 15, 2022

felixfontein commented Mar 2, 2022

patchback bot commented Mar 10, 2022 •

edited

Loading

felixfontein commented Mar 10, 2022

patchback bot commented Mar 10, 2022 •

edited

Loading

earchibald-lv commented Mar 15, 2022 •

edited

Loading

earchibald-lv commented Mar 15, 2022

earchibald-lv commented Mar 15, 2022

mkjpryor commented Mar 15, 2022 •

edited

Loading

earchibald-lv commented Mar 15, 2022

mkjpryor commented Mar 15, 2022

mkjpryor commented Mar 15, 2022

mkjpryor commented Mar 15, 2022

earchibald-lv commented Mar 15, 2022 •

edited

Loading

mkjpryor commented Mar 15, 2022

felixfontein commented Mar 16, 2022

felixfontein commented Mar 16, 2022

Allow complex values in variables parameter of terraform module #4281

Allow complex values in variables parameter of terraform module #4281

Conversation

wmudge commented Feb 23, 2022

SUMMARY

ISSUE TYPE

COMPONENT NAME

ansibullbot commented Feb 23, 2022

felixfontein left a comment

Choose a reason for hiding this comment

felixfontein Feb 23, 2022

Choose a reason for hiding this comment

felixfontein Mar 1, 2022

Choose a reason for hiding this comment

wmudge Mar 1, 2022

Choose a reason for hiding this comment

felixfontein Mar 1, 2022

Choose a reason for hiding this comment

earchibald-lv Mar 15, 2022

Choose a reason for hiding this comment

felixfontein commented Mar 2, 2022

patchback bot commented Mar 10, 2022 • edited Loading

Backport to stable-3: 💚 backport PR created

felixfontein commented Mar 10, 2022

patchback bot commented Mar 10, 2022 • edited Loading

Backport to stable-4: 💚 backport PR created

earchibald-lv commented Mar 15, 2022 • edited Loading

earchibald-lv commented Mar 15, 2022

earchibald-lv commented Mar 15, 2022

mkjpryor commented Mar 15, 2022 • edited Loading

earchibald-lv commented Mar 15, 2022

mkjpryor commented Mar 15, 2022

mkjpryor commented Mar 15, 2022

mkjpryor commented Mar 15, 2022

earchibald-lv commented Mar 15, 2022 • edited Loading

mkjpryor commented Mar 15, 2022

felixfontein commented Mar 16, 2022

felixfontein commented Mar 16, 2022

patchback bot commented Mar 10, 2022 •

edited

Loading

patchback bot commented Mar 10, 2022 •

edited

Loading

earchibald-lv commented Mar 15, 2022 •

edited

Loading

mkjpryor commented Mar 15, 2022 •

edited

Loading

earchibald-lv commented Mar 15, 2022 •

edited

Loading